Skip to content

[Support] Add missing LLVM_ABI annotations in Atomic.h #152768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 8, 2025

Conversation

mstorsjo
Copy link
Member

@mstorsjo mstorsjo commented Aug 8, 2025

This fixes building LLD for mingw targets with
LLVM_BUILD_LLVM_DYLIB_VIS=ON with Clang. This has been missed for other platforms, as those platforms have LLVM_THREADING_USE_STD_CALL_ONCE=1 in llvm/Support/Threading.h, while it ends up set to 0, using CompareAndSwap() and MemoryFence() instead, for mingw targets.

@mstorsjo
Copy link
Member Author

mstorsjo commented Aug 8, 2025

CC @andrurogerz

@llvmbot
Copy link
Member

llvmbot commented Aug 8, 2025

@llvm/pr-subscribers-llvm-support

Author: Martin Storsjö (mstorsjo)

Changes

This fixes building LLD for mingw targets with
LLVM_BUILD_LLVM_DYLIB_VIS=ON. This has been missed for other platforms, as those platforms have LLVM_THREADING_USE_STD_CALL_ONCE=1 in llvm/Support/Threading.h, while it ends up set to 0, using CompareAndSwap() and MemoryFence() instead, for mingw targets.


Full diff: https://github.com/llvm/llvm-project/pull/152768.diff

1 Files Affected:

  • (modified) llvm/include/llvm/Support/Atomic.h (+5-4)
diff --git a/llvm/include/llvm/Support/Atomic.h b/llvm/include/llvm/Support/Atomic.h
index a8445fddc1a85..4dcef19359ce5 100644
--- a/llvm/include/llvm/Support/Atomic.h
+++ b/llvm/include/llvm/Support/Atomic.h
@@ -17,6 +17,7 @@
 #ifndef LLVM_SUPPORT_ATOMIC_H
 #define LLVM_SUPPORT_ATOMIC_H
 
+#include "llvm/Support/Compiler.h"
 #include "llvm/Support/DataTypes.h"
 
 // Windows will at times define MemoryFence.
@@ -26,16 +27,16 @@
 
 namespace llvm {
   namespace sys {
-    void MemoryFence();
+    LLVM_ABI void MemoryFence();
 
 #ifdef _MSC_VER
     typedef long cas_flag;
 #else
     typedef uint32_t cas_flag;
 #endif
-    cas_flag CompareAndSwap(volatile cas_flag* ptr,
-                            cas_flag new_value,
-                            cas_flag old_value);
+    LLVM_ABI cas_flag CompareAndSwap(volatile cas_flag* ptr,
+                                     cas_flag new_value,
+                                     cas_flag old_value);
   }
 }
 

Copy link

github-actions bot commented Aug 8, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

This fixes building LLD for mingw targets, with
LLVM_BUILD_LLVM_DYLIB_VIS=ON. This has been missed for other
platforms, as those platforms have LLVM_THREADING_USE_STD_CALL_ONCE=1
in llvm/Support/Threading.h, while it ends up set to 0, using
CompareAndSwap() and MemoryFence() instead, for mingw targets.
@andrurogerz
Copy link
Contributor

lgtm, thanks!

@mstorsjo
Copy link
Member Author

mstorsjo commented Aug 8, 2025

@andrurogerz As for why this had been missed; I had the understanding that the current annotations were made with some automated tool - shouldn't that have caught these declarations? They should be available and exported in all builds, even if most users (with LLVM_THREADING_USE_STD_CALL_ONCE=1) don't end up using them?

@vgvassilev vgvassilev requested a review from compnerd August 8, 2025 17:56
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@andrurogerz
Copy link
Contributor

As for why this had been missed; I had the understanding that the current annotations were made with some automated tool - shouldn't that have caught these declarations?

Correct, the annotations are automatically added by the IDS tool, but it is run on each header individually and this is one was missed. There will likely be more that pop-up over the coming weeks.

@mstorsjo
Copy link
Member Author

mstorsjo commented Aug 8, 2025

FWIW, I may have spoken too soon, re

This fixes building LLD for mingw targets with LLVM_BUILD_LLVM_DYLIB_VIS=ON.

As in, it does fix it, if building with Clang, but it still mostly uses __attribute__((visibility("default"))) and hidden in that configuration (and doesn't work as is out of the box in builds with GCC) - it did seem a bit too good to be true. It will require a number of further tweaks to take these annotations fully in use in the form of dllexport/import for mingw - I'll try it out soon.

But this fix is one step on the way in any case.

@mstorsjo mstorsjo merged commit 08873be into llvm:main Aug 8, 2025
9 checks passed
@mstorsjo mstorsjo deleted the llvm-abi-atomic branch August 8, 2025 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants